[code-simplifier] refactor: dedupe isRestEndpoint helper into github_api_helpers.cjs - #49261
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the implementation label and has 0 new lines of code in business logic directories (threshold: 100). |
|
✅ Test Quality Sentinel completed test quality analysis. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
✅ PR Code Quality Reviewer completed the code quality review. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
There was a problem hiding this comment.
Pull request overview
Centralizes the duplicated REST endpoint type guard in the shared GitHub API helper module.
Changes:
- Exports
isRestEndpointfromgithub_api_helpers.cjs. - Replaces duplicate implementations with shared imports.
- One JSDoc placement issue requires correction.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/github_api_helpers.cjs |
Adds and exports the shared type guard. |
actions/setup/js/add_workflow_run_comment.cjs |
Imports the shared helper. |
actions/setup/js/add_reaction_and_edit_comment.cjs |
Imports the shared helper. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
| /** | ||
| * Type guard for a resolved REST endpoint descriptor, shared by handlers that | ||
| * choose between a REST call ({route, params}) and a GraphQL/discussion path. | ||
| * @param {unknown} endpoint | ||
| * @returns {endpoint is { route: string, params: Record<string, unknown> }} |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review 🧠
Applied /codebase-design — requesting one fix before merge.
📋 Key Themes & Highlights
Issue
- Orphaned JSDoc: inserting
isRestEndpointbetween the existingresolveTopLevelDiscussionCommentIdJSDoc block and its function body causes JS tooling to attribute those docs to the wrong function. See inline comment for the fix.
Positive Highlights
- ✅ Clean deduplication — identical function bodies collapsed into a single canonical location
- ✅ Correct module:
github_api_helpers.cjsis the right home for this type guard - ✅ Export added in alphabetical order
- ✅ PR is well-scoped with no behavior change and passing tests
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 21.3 AIC · ⊞ 7K
Comment /matt to run again
| * @param {string|null|undefined} commentNodeId - The node_id of the triggering comment | ||
| * @returns {Promise<string|null|undefined>} The node ID to use as replyToId (parent if reply, otherwise the original) | ||
| */ | ||
| /** |
There was a problem hiding this comment.
[/codebase-design] The JSDoc block for resolveTopLevelDiscussionCommentId (lines 146–155) is now orphaned — isRestEndpoint was inserted between it and the function it documents. JS tooling (IDEs, JSDoc, TypeDoc) will associate those @param/@returns docs with isRestEndpoint instead.
💡 Fix: reorder so each JSDoc immediately precedes its function
/**
* Type guard for a resolved REST endpoint descriptor ...
* `@param` {unknown} endpoint
* `@returns` {endpoint is { route: string, params: Record<string, unknown> }}
*/
function isRestEndpoint(endpoint) { ... }
/**
* Resolves the top-level parent comment node ID ...
* `@param` {Object} github
* `@param` {string|null|undefined} commentNodeId
* `@returns` {Promise<string|null|undefined>}
*/
async function resolveTopLevelDiscussionCommentId(github, commentNodeId) { ... }@copilot please address this.
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. The The new
|
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Review: dedupe isRestEndpoint — one blocking issue
Overall: The refactor is correct — the function body is identical across both callers and the shared placement in github_api_helpers.cjs is the right home for it. However there is one structural bug introduced by the insertion order.
Blocking issue: isRestEndpoint was inserted between the JSDoc block that documents resolveTopLevelDiscussionCommentId and the function itself (lines 155→156→162). This severs the doc comment from its function; TypeScript language services and doc generators will incorrectly attribute it to isRestEndpoint.
Fix: move isRestEndpoint + its JSDoc to after resolveTopLevelDiscussionCommentId, just before the module.exports block, or swap the order so the existing JSDoc sits immediately above its function.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 20.4 AIC · ⊞ 5.3K
| * @param {unknown} endpoint | ||
| * @returns {endpoint is { route: string, params: Record<string, unknown> }} | ||
| */ | ||
| function isRestEndpoint(endpoint) { |
There was a problem hiding this comment.
JSDoc detachment bug: The isRestEndpoint function was inserted immediately after the closing */ of the resolveTopLevelDiscussionCommentId JSDoc block, disconnecting that doc from its function. TypeScript/IDEs will associate the stray JSDoc (lines 148–155) with isRestEndpoint instead of resolveTopLevelDiscussionCommentId.
Fix: move isRestEndpoint (and its own JSDoc) to after resolveTopLevelDiscussionCommentId, or keep the existing doc immediately above its function.
@copilot please address this.
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Verdict: approve with a minor doc-placement nit
Clean, faithful dedup of isRestEndpoint into the shared helpers module — no behavior change, exports updated correctly.
Themes
- The moved function's logic is byte-for-byte identical to both prior copies (verified `typeof === "object" && !== null && "route" in && "params" in" checks match).
- One low-severity issue: the pre-existing JSDoc for
resolveTopLevelDiscussionCommentIdwas left above the newly insertedisRestEndpoint, orphaning both functions' documentation. Flagged inline. - No correctness, security, or performance concerns in the diff.
🔎 Code quality review by PR Code Quality Reviewer · auto · 63.5 AIC · ⊞ 7.8K
Comment /review to run again
| * @param {string|null|undefined} commentNodeId - The node_id of the triggering comment | ||
| * @returns {Promise<string|null|undefined>} The node ID to use as replyToId (parent if reply, otherwise the original) | ||
| */ | ||
| /** |
There was a problem hiding this comment.
JSDoc placement bug: the pre-existing doc comment for resolveTopLevelDiscussionCommentId now sits above the new isRestEndpoint function instead of its intended target.
💡 Details
The comment block describing commentNodeId/return value was left directly above the inserted isRestEndpoint function, so isRestEndpoint inherits an unrelated JSDoc (talking about github/commentNodeId params it does not have), while resolveTopLevelDiscussionCommentId loses its own documentation entirely. This breaks IDE tooltips/intellisense for both functions.
Fix: move the new isRestEndpoint JSDoc above its own function, and keep the old JSDoc directly above resolveTopLevelDiscussionCommentId.
|
@copilot run pr-finisher skill |
…CommentId by relocating isRestEndpoint Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in the latest commit. Moved |
|
🎉 This pull request is included in a new release. Release: |
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Summary
Deduplicates the
isRestEndpointtype guard function, which was previously defined identically in two handler files, into a single shared implementation ingithub_api_helpers.cjs.Changes
actions/setup/js/github_api_helpers.cjs: Added theisRestEndpointtype guard (checks that a value is an object withrouteandparamsproperties) and exported it from the module.actions/setup/js/add_reaction_and_edit_comment.cjs: Removed the local duplicateisRestEndpointdefinition; now imports it fromgithub_api_helpers.cjs.actions/setup/js/add_workflow_run_comment.cjs: Removed the local duplicateisRestEndpointdefinition; now imports it fromgithub_api_helpers.cjs.Notes
eecb5a279) relocated the new function's placement withingithub_api_helpers.cjsto restore correct JSDoc-to-function association forresolveTopLevelDiscussionCommentId.Impact